Add Right/left/Cross/Indexed Joins and Implement Table Edit Accumulator #596

VinaiRachakonda · 2021-10-21T16:23:38Z

Add functionality for updates to work with Indexed Joins
Implement a Table Edit Accumulator that writes changes at Close time
Fix some undefined test cases
Add functionality for Update Join w/ Right,Left,Cross joins

…ongterm

VinaiRachakonda · 2021-10-25T18:36:27Z

memory/table_editor.go

+
+// Get implements the tableEditAccumulator interface.
+func (k *keylessTableEditAccumulator) Get(value sql.Row) (sql.Row, bool, bool, error) {
+	// Note: Keyless tables do not have to return an accurate answer here as any given row can be inserted or deleted


I don't like this tbh but I also don't like adding conditional logic in table.go for checking this condition. Maybe implement a keyless table implementation as well?

Just punt for now

zachmu

Think this is ready to go, nice work.

The main request I have is to follow up after this goes in to do something different for the JoinSorter node.

zachmu · 2021-10-25T23:53:09Z

enginetest/update_queries.go

@@ -261,6 +261,110 @@ var UpdateTests = []WriteQueryTest{
 			sql.NewRow(1, 1, 32, 31, 32, 33, 34),
 		},
 	},
+	{
+		WriteQuery:          `UPDATE one_pk INNER JOIN two_pk on one_pk.pk = two_pk.pk1 INNER JOIN two_pk a1 on one_pk.pk = two_pk.pk2 SET two_pk.c1 = two_pk.c1 + 1`, // cross join


I'm not verifying these query results by hand, make sure you have (or tested them on mysql)

This isn't a cross join (a cross join has no join condition)(

zachmu · 2021-10-25T23:55:41Z

enginetest/update_queries.go

+		},
+	},
+	{
+		WriteQuery:          `UPDATE othertable INNER JOIN tabletest on othertable.i2=3 and tabletest.i=3 SET othertable.s2 = 'fourth'`, // cross join


This is a cross join but not phrased as on

A cross join has no join condition

This just reduces to a cross join because the analyzer realizes that none of the join predicates are actually join predicates to moves them to the filter.

You should keep this test and add another one that is an explicit cross join

You should make sure you have tests for the implicit cross join, e.g.

update table1, table2 set...

zachmu · 2021-10-25T23:59:09Z

enginetest/update_queries.go

-		ExpectedErr: sql.ErrUnsupportedFeature,
-	},
-}
+var UpdateErrorTests = []QueryErrorTest{}


Delete this now it's empty?

zachmu · 2021-10-26T00:02:32Z

memory/table.go

+		return err
+	}
+
+	t.table = tbl


Shouldn't need to do this, you won't be reusing this object after close is called

zachmu · 2021-10-26T00:03:47Z

memory/table.go

+		for _, i := range pkColIdxes {
+			vals[i] = row[pkColIdxes[i]]
+		}
+		return sql.NewUniqueKeyErr(fmt.Sprint(vals), true, partitionRow)


Do we have engine tests for this?

(We don't, they wouldn't have passed before)

zachmu · 2021-10-26T00:20:30Z

memory/table_editor.go

+	return nil
+}
+
+// insertHelper deletes a row from a keyless table, if it exists.


Comment wrong

zachmu · 2021-10-26T00:21:40Z

memory/table_editor.go

+// insertHelper deletes a row from a keyless table, if it exists.
+func (k *keylessTableEditAccumulator) insertHelper(ctx *sql.Context, table *Table, row sql.Row) error {
+	key := string(table.keys[table.insert])
+	table.insert++


table.insert should be called table.insertPartIdx or similar

zachmu · 2021-10-26T00:21:54Z

memory/table_editor.go

+func (k *keylessTableEditAccumulator) insertHelper(ctx *sql.Context, table *Table, row sql.Row) error {
+	key := string(table.keys[table.insert])
+	table.insert++
+	if table.insert == len(table.keys) {


table.keys should be called table.partitionKeys

zachmu · 2021-10-26T00:22:44Z

sql/analyzer/assign_update_join.go

-		if jn == nil {
-			return n, nil
+		if unsupported {
+			return nil, sql.ErrUnsupportedFeature.New()


Is this still possible? Didn't you remove all the test cases that might trigger this?

zachmu · 2021-10-26T00:47:29Z

sql/plan/indexed_join_sorter.go

+
+// IndexedJoinSorter is used to sort the output of an IndexedJoin to fit the original join order. Note that this
+// occurs because IndexedJoins use join optimization logic that changes the original given join order.
+type IndexedJoinSorter struct {


So I don't like this solution, although you can check it in for the time being as long as you follow up with a fix.

This is basically just a projection (mapping from one column order to another), not a new kind of node that we should have to account for separately everywhere (which will be prone to bugs). I think the only reason this works is that it's applied in the join optimization step, which happens last. Otherwise the new node type would cause issues (lots of case statements to clean up).

So you have two choices: either replace this with a straight projection (easy enough).

Or put the projection into the UpdateJoin node itself (probably better).

But either way, definitely don't introduce a new node type here.

Yeah 100% makes sense. Will followup with a new pr.

VinaiRachakonda added 20 commits October 18, 2021 11:01

merge indexed join stuff

11840a3

working prototype for the indexed join

71e2916

some cleanup

7d5a573

uncomment

798d71f

Merge branch 'main' into vinai/additional-update-join-support

9c86eb8

fix the sorter analyzer step. Need to get memory table editor to work

322bd67

really stupid implementation of an edit accumulator

810003a

basic implementation

86cb384

rewrote editor to get all tests passing for now:

822267e

tests pass with split

b722d9e

formatting and still passing tests

f0f9648

cleanup and undo some dumd goland refactors

bf3d11c

cleanup/remove flakiness

f92cbce

formatting and cleaning up sloppy code

b8b1337

cleanup edits interface

00c0ecd

clean up interface

08a2262

realized bug is that table needs to be an implicity sorted map

ba837b8

this is sorting logic. Checking it in for now but not useful in the l…

0c2e19b

…ongterm

removed sorting implementaiton and fixed the flaky test

db56f4e

merge

15d8a24

VinaiRachakonda changed the title ~~[WIP] Updated Join with Indexes~~ Updated Join with Indexes and Implement Table Edit Accumulator Oct 22, 2021

final nit

6bbf376

VinaiRachakonda requested a review from zachmu October 22, 2021 18:10

VinaiRachakonda added 2 commits October 22, 2021 18:30

working solution, needs more tests

0600ab2

more tests

d4e7fea

VinaiRachakonda commented Oct 25, 2021

View reviewed changes

cleanup and revert single query

5358690

VinaiRachakonda changed the title ~~Updated Join with Indexes and Implement Table Edit Accumulator~~ Add Right/left/Cross/Indexed Joins and Implement Table Edit Accumulator Oct 25, 2021

zachmu approved these changes Oct 26, 2021

View reviewed changes

Pr review comments

55baecc

VinaiRachakonda merged commit 950fb2e into main Oct 26, 2021

Hydrocharged deleted the vinai/additional-update-join-support branch December 8, 2021 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Right/left/Cross/Indexed Joins and Implement Table Edit Accumulator #596

Add Right/left/Cross/Indexed Joins and Implement Table Edit Accumulator #596

VinaiRachakonda commented Oct 21, 2021 •

edited

Loading

VinaiRachakonda Oct 25, 2021

zachmu Oct 26, 2021

zachmu left a comment

zachmu Oct 25, 2021

zachmu Oct 25, 2021

zachmu Oct 25, 2021

zachmu Oct 25, 2021

zachmu Oct 25, 2021

zachmu Oct 26, 2021

zachmu Oct 26, 2021

zachmu Oct 26, 2021

zachmu Oct 26, 2021

zachmu Oct 26, 2021

zachmu Oct 26, 2021

zachmu Oct 26, 2021

VinaiRachakonda Oct 26, 2021

Add Right/left/Cross/Indexed Joins and Implement Table Edit Accumulator #596

Add Right/left/Cross/Indexed Joins and Implement Table Edit Accumulator #596

Conversation

VinaiRachakonda commented Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinaiRachakonda commented Oct 21, 2021 •

edited

Loading